fix: stop asserting CPython's recursion limit in the hostile-JSON guards - #78
Merged
Merged
Conversation
The free-threaded 3.14t matrix row went red on three tests in test_resilience_hardening.py. Nothing in recon changed; CPython did. The free-threaded build parses the 100,000-deep fixture without exhausting its C stack, where the GIL build raises RecursionError, so guards that exist to catch that error had nothing to catch. The tests were asserting the interpreter's mechanism rather than recon's contract. The audit finding they encode is real and stays covered: the cache loaders and the CT providers' resp.json() guard caught only ValueError, so a deeply-nested payload escaped the degrade path. That is a statement about which except clause recon needs, and it is only meaningful on an interpreter that raises in the first place. Two options were rejected. Lowering sys.setrecursionlimit does not work: it has not governed the C scanner since 3.12, verified locally at depths of 50, 200, and 1,000, all of which parse under a limit of 100. Probing deeper to force the error is worse than useless, because a payload deep enough to exhaust a free-threaded stack leaves a nested list CPython can crash while deallocating, trading a skipped assertion for a segfault. So the module probes once, safely, at exactly the depth it already uses, and gates on the result. The two CT provider tests skip where no RecursionError can occur, with a reason that says why rather than naming a version. The cache test keeps its behavioural assertion running everywhere. Only its sanity precondition is interpreter-dependent; a 100,000-deep array is still not a cache record and must still be refused, so cache_get returning None is asserted on every build. Coverage note: on free-threaded builds the provider-local RecursionError degrade path is now unexercised. That path cannot be reached there, so this records the gap rather than hiding it behind an assertion that cannot fire. Full check.py passes all 27 stages.
There was a problem hiding this comment.
Pull request overview
Updates the hostile-input resilience tests to stop asserting CPython’s recursion-limit behavior directly, and instead gate the RecursionError-specific assertions based on whether the interpreter actually errors on the existing 100,000-deep JSON fixture. This keeps recon’s “degrade instead of crashing” contract covered while accommodating free-threaded builds whose JSON parser no longer raises.
Changes:
- Added an interpreter probe (
_parser_exhausts_on_nesting) and a skip marker to conditionally run RecursionError-specific assertions. - Modified the poisoned-cache test to always assert recon’s behavioral contract (
cache_get(...) is None), while only assertingRecursionErrorwhen the parser actually raises. - Added skip gating to CT-provider degradation tests that depend on
resp.json()raisingRecursionError.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+84
to
+94
| _PARSER_EXHAUSTS_ON_NESTING = _parser_exhausts_on_nesting() | ||
|
|
||
| # Guards that can only fire once the parser itself gives up. Where it does not, | ||
| # the fixture is ordinary valid JSON and there is no RecursionError to catch. | ||
| _REQUIRES_PARSER_EXHAUSTION = pytest.mark.skipif( | ||
| not _PARSER_EXHAUSTS_ON_NESTING, | ||
| reason=( | ||
| "this interpreter parses 100,000-deep JSON without exhausting its C stack, " | ||
| "so no RecursionError exists to degrade from" | ||
| ), | ||
| ) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The free-threaded
3.14tmatrix row went red on three tests intest_resilience_hardening.py. Nothing in recon changed; CPython did. The free-threaded build parses the 100,000-deep fixture without exhausting its C stack, where the GIL build raisesRecursionError— so guards that exist to catch that error had nothing to catch.The tests were asserting the interpreter's mechanism rather than recon's contract. The audit finding they encode is real and stays covered: the cache loaders and the CT providers'
resp.json()guard caught onlyValueError, so a deeply-nested payload escaped the degrade path. That is a statement about whichexceptclause recon needs, and it is only meaningful on an interpreter that raises in the first place.Two options rejected
Lower
sys.setrecursionlimit. Does not work — it has not governed the C scanner since 3.12. Verified locally under a limit of 100:Probe deeper to force the error. Worse than useless. A payload deep enough to exhaust a free-threaded stack leaves a nested list CPython can crash while deallocating, trading a skipped assertion for a segfault.
What this does instead
Probe once, safely, at exactly the depth already in use, and gate on the result. The skip reason states the condition rather than naming a version, so it stays correct if the limits move again.
The cache test keeps its behavioural assertion running everywhere — only its sanity precondition is interpreter-dependent. A 100,000-deep array is still not a cache record and must still be refused, so
cache_get(...) is Noneis asserted on every build.Coverage note
On free-threaded builds the provider-local
RecursionErrordegrade path is now unexercised. That path cannot be reached there, so this records the gap rather than hiding it behind an assertion that cannot fire. If a future CPython makes the depth reachable again, the probe re-enables the strict tests automatically.Verification
uv run python scripts/check.py— all 27 stages passuv run pytest tests/test_resilience_hardening.py -m hostile_input -q— 31 passed on the GIL build, strict path exercised3.14twas green on6413bbd,d69e224, and8a0536b, and it first failed on a docs-only PR